Skip to content

Fix skip condition of gh13082.phpt #14922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jul 11, 2024

The test failure is not particularly related to Travis, but rather is caused by the GD font file to only be suitable for platforms where int stores 32bit values in little endian byte order. This platform dependence is documented in the source code[1]. Thus we fix the skip condition and skip reason accordingly.

An alternative would be to dynamically create the font file just before running the test, but that appears to be overkill.

[1]

php-src/ext/gd/gd.c

Lines 545 to 556 in d59691c

/* Only supports a architecture-dependent binary dump format
* at the moment.
* The file format is like this on machines with 32-byte integers:
*
* byte 0-3: (int) number of characters in the font
* byte 4-7: (int) value of first character in the font (often 32, space)
* byte 8-11: (int) pixel width of each character
* byte 12-15: (int) pixel height of each character
* bytes 16-: (char) array with character data, one byte per pixel
* in each character, for a total of
* (nchars*width*height) bytes.
*/


Note that this is a follow up to #13208.

The skip reason might be improved; I've kept it as short as possible for now.

The test failure is not particularly related to Travis, but rather is
caused by the GD font file to only be suitable for platforms where
`int` stores 32bit values in little endian byte order.  This platform
dependence is documented in the source code[1].  Thus we fix the skip
condition and skip reason accordingly.

An alternative would be to dynamically create the font file just before
running the test, but that appears to be overkill.

[1] <https://github.com/php/php-src/blob/d59691c02fa77b65855ebbcd5f50aaac034ab75d/ext/gd/gd.c#L545-L556>
@@ -3,7 +3,7 @@ GH-13082 - imagefontwidth/height unexpectedly throwing an exception on a valid G
--EXTENSIONS--
gd
--SKIPIF--
<?php if (getenv('TRAVIS')) die('skip Currently fails on Travis'); ?>
<?php if (pack('i', 0x01020304) !== "\x04\x03\x02\x01") die('skip unsupported platform'); ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to be clear about "what" the platform does not support.

Suggested change
<?php if (pack('i', 0x01020304) !== "\x04\x03\x02\x01") die('skip unsupported platform'); ?>
<?php if (pack('i', 0x01020304) !== "\x04\x03\x02\x01") die('skip big endian is unsupported'); ?>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but it's not only about the endianess, but also int must be 32bit. Maybe the latter is pretty much the case everywhere nowadays, but that might change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I overlooked that. It is impossible to explain this in detail in one sentence.

@cmb69 cmb69 closed this in 02a60be Jul 14, 2024
@cmb69 cmb69 deleted the cmb/gh13082 branch July 14, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants